Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't log error when evaluating aliases array #104

Merged
merged 4 commits into from
Mar 19, 2024
Merged

Conversation

louisheath
Copy link
Contributor

@louisheath louisheath commented Mar 19, 2024

when parsing aliases we

  • accept an array of source references
  • evaluate each source reference, attempting to parse its result as a string
  • if that fails, evaluate it again, attempting to parse its result as an array of strings

this flow results in an error being logged.
this PR prevents that error log

@louisheath louisheath changed the title Don't error when evaluating arrays Don't log error when evaluating aliases array Mar 19, 2024
expr/js_eval.go Outdated
return resultValue, nil

case isArray(result):
fmt.Fprintf(os.Stdout, "\n Source %s evaluates to an array. Handling separately\n", source)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to log anything here? I'm not sure we should. I think we'll only hit this when we try to evaluate an alias JS expression as a string but it returns an array, and the user doesn't need to know about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're catering for our known path but a user could write all sorts of bad configuration , e.g.
name: $.aliases

in this case we don't error but it's worth making some level of noise imo

@pipt pipt merged commit 7858b5c into master Mar 19, 2024
1 check passed
@pipt pipt deleted the louis/cat-34-handle-log branch March 19, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants